Move test harness to separate package #110
Conversation
Allow both dotenvy and dotenvy_macro to make use of the test harness
The [`should_panic_without_expect`](https://rust-lang.github.io/rust-clippy/master/index.html#/should_panic_without_expect) pedantic lint is failing CI. The lint is not that useful given our small tests.
allan2
left a comment
allan2
left a comment
There was a problem hiding this comment.
I can see this being very useful in creating test files, test text, and testing more complicated cases.
I would prefer if the usage of custom assert functions were minimized. Fallible functions such as Efb::add_env_file should return Result. The tests themselves can be written a Result signature as well. A custom error type may be helpful here.
test_util/src/envfile.rs
Outdated
| } | ||
|
|
||
| /// Add a string without a newline | ||
| pub fn add_str(&mut self, s: &str) -> &mut Self { |
There was a problem hiding this comment.
push_str may be more conventional.
There was a problem hiding this comment.
If we change to push or append then the other "add" functions should follow suit.
So we'd have:
push_varspush_varpush_strpush_strlnpush_bytespush_byte
Or the append version.
There was a problem hiding this comment.
I think less is better. I think of Efb as a wrapper around a vector of bytes, with one convenience function, add_key_value.
We shouldn't try to do too much by supporting both byte functions and string functions.
My suggestion is to implement some of the Vec functions:
add_byte -> push
add_bytes -> extend_from_slice
add_str -> extend_from_slice(s.as_bytes())
add_strln -> can add a string and push the newline char
And implement the convenience functions:
add_key_value -> add_var
add_vars -> use a for loop with add_var
Test writers can use the Vec API and the string-to-byte conversions they are already familiar with.
There was a problem hiding this comment.
I agree, simpler is better. The majority of use cases would be pushing &str, so I do think keeping that as push_str is worthwhile. Therefore, push_bytes makes sense as a corollary.
add_vars does have a performance improvement to looping over add_var. Looping will perform multiple allocations. BUT, I will concede that the majority of new tests will only have 3 vars max in an env file.
So I will remove it.
test_util/src/envfile.rs
Outdated
| /// | ||
| /// Represented as bytes to allow for advanced manipulation and BOM testing. | ||
| #[derive(Debug, Default)] | ||
| pub struct EnvFileBuilder { |
There was a problem hiding this comment.
I'm not a fan of the Builder suffix, since it's too commonly used in the other context, i.e., describing the API design. I would suggest EnvFileAdv, with "Adv" short for Advanced. Perhaps you can think of something better.
There was a problem hiding this comment.
Some options:
EnvFileAdvCreatorEnvFileMakerAdvancedEnvFile
I quite like the last one. The struct can be passed into init_with_env_file and add_env_file, so semantically it makes sense.
There was a problem hiding this comment.
I am good with either AdvancedEnvFile or AdvEnvFile. I personally prefer shorter names; they are more adventurous (AdventurousEnvFile?)
There was a problem hiding this comment.
Efb is such a good acronym though. EnvFileBuilder has grown on me!
There was a problem hiding this comment.
I am currently simplifying this crate. Removing the "default" code I have come to realise this should just be called EnvFileContents and be the primary way one is constructed.
A String, &str, Vec<u8>, &[u8] or EnvFileBuilder, can already be passed to TestEnv::add_env_file. So why not just call this EnvFileContents as it will be the only way of constructing one besides just using the primitive types. Plus that's exactly what it is.
efc is a pretty good acronym too ;)
Understood, I'll go ahead and add an error type. You're right for wanting the failed states to be as descriptive as possible. When these utilities were part of the test harness, it made sense to have custom panic messages. Now it is a separate package: errors are far more appropriate.
Perhaps if we removed the default |
To match env_var
- Clone - PartialEq - Eq - PartialEq to String and &str - PartialEq to Vec<u8> and &[u8] - AsRef<[u8]>
|
@sonro I added my comments on Would you rather wait for that before updating this PR further? |
The new API can still be tested with these utilities too! #[test]
fn existing_var_no_env_file() {
let mut testenv = TestEnv::init();
testenv.add_env_var("TEST_KEY", "test_val");
test_in_env(&testenv, || {
let safe_vars = dotenvy::load_all().unwrap(); // or whatever the api ends up being
let actual = safe_vars.get("TEST_KEY").unwrap();
assert_eq!("test_val", actual");
});
}Plus we need it to thoroughly test the existing API - deprecated or not. We shouldn't mind unsafety within the test harness as it already uses a mutex for thread safety. |
Let users of the test utils customize their own defaults
To better match Rust API guidelines
|
|
2 participants
The test utilities, proposed in #58, shouldn't just be in one test suite. The harness can, and should, be used across the project. It could even be used outside of
dotenvyentirely.This PR creates a new workspace member
dotenvy_test_util- which contains the test harness along with its documentation.The new test suite itself is nearly finished, see #111.